fix: handle Python paths with spaces correctly #445
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReworks Python command parsing and validation to safely handle path-like strings and spaces, switches execSync -> execFileSync with parsed args, expands allowed Python path patterns and runtime verification, and adds multi-shell detection plus shell-aware escaping and command builders for terminal login and Claude flows. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Renderer (UI)
participant Main as Main (IPC handlers)
participant Detector as Python Detector
participant ShellUtil as Shell Utils
participant ShellProc as Terminal Process
rect rgba(120,160,255,0.06)
Note over UI,ShellProc: Shell-aware terminal login / Claude invocation with Python validation
end
UI->>Main: request startTerminal/login (env, token, optional shellPath)
Main->>ShellUtil: detectShellType(shellPath)
ShellUtil-->>Main: ShellType
Main->>Detector: parsePythonCommand(commandStr)
Detector-->>Main: {cmd, args} and validation result
Main->>ShellUtil: escape token/args for ShellType
ShellUtil-->>Main: escaped command fragments
Main->>ShellProc: spawn shell with assembled shell-specific command
ShellProc-->>Main: started / stdout / exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (3)apps/frontend/src/**/*.{ts,tsx,jsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
apps/frontend/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
apps/frontend/**/*.{ts,tsx}⚙️ CodeRabbit configuration file
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
All contributors have signed the CLA. Thank you! |
Summary of ChangesHello @abe238, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical issue where Python paths containing spaces, particularly those found in Electron application data directories on macOS and Windows, were causing Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the issue of handling Python paths with spaces, which was causing errors on platforms like macOS. The core logic change in parsePythonCommand to prioritize path structure over file existence is a solid fix. Additionally, switching to execFileSync from execSync is a great improvement for both security and robustness, preventing shell interpretation issues. The new regex patterns for Electron's userData paths on both macOS and Windows are also a necessary addition. I've included a couple of suggestions to consolidate similar regular expressions for improved maintainability. Overall, this is a well-executed fix.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/frontend/src/main/python-detector.tsapps/frontend/src/main/python-env-manager.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/python-env-manager.tsapps/frontend/src/main/python-detector.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/python-env-manager.tsapps/frontend/src/main/python-detector.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/python-env-manager.tsapps/frontend/src/main/python-detector.ts
🪛 GitHub Actions: CI
apps/frontend/src/main/python-env-manager.ts
[error] 205-205: TypeScript error: Cannot find name 'parsePythonCommand'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (4)
apps/frontend/src/main/python-detector.ts (3)
211-213: Excellent documentation enhancement.The added emphasis on handling paths with spaces is clear and directly addresses the root cause described in the PR. This will help prevent future regressions.
237-255: Core fix correctly implements path-with-spaces handling.The reordering of checks is the key improvement:
- Path separator detection now happens before existence checks
- Paths with separators are returned as-is without splitting on spaces
- This correctly handles venv paths that don't exist yet during setup
The logic is sound and directly addresses the root cause of the ENOENT errors.
306-309: Pattern coverage is correct—no changes needed.The added patterns accurately match Electron userData paths:
- macOS:
/Users/<user>/Library/Application Support/<app>/is the standard userData location; non-standard home directories are not a practical concern.- Windows:
AppData\Roaming\<app>\is the correct userData location;AppData\Localis used only for system Python installations (already handled by line 318).- App name matching:
[^/]+and[^\\]+correctly capture all app names while preventing directory traversal.The generic patterns at lines 313–315 provide fallback coverage for Linux and other venv naming variations.
apps/frontend/src/main/python-env-manager.ts (1)
204-210: Correct fix for shell interpretation issues with spaces.The change from
execSynctoexecFileSyncwithshell: falseis the right approach:
- Prevents the shell from splitting paths at spaces
- Uses
parsePythonCommandto correctly parse the command- Maintains the same 5-second timeout for safety
This change works correctly once the missing import is added (see previous comment).
Note: This comment assumes the missing
parsePythonCommandimport will be added as suggested in the previous review comment.
- Updated parsePythonCommand to check for path separators BEFORE existsSync to properly handle paths that may not exist yet - Added macOS Application Support and Windows AppData patterns to ALLOWED_PATH_PATTERNS for Electron userData venv paths - Changed python-env-manager to use execFileSync instead of execSync to avoid shell interpretation issues with spaces Fixes spawn ENOENT errors when venv Python path contains spaces, e.g., /Users/user/Library/Application Support/App/python-venv/bin/python 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: Abe Diaz (@abe238) <[email protected]>
abe238
force-pushed
the
fix/python-path-spaces
branch
from
54bb603 to
890704c
Compare
AlexMadera
left a comment
AlexMadera
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: ✅ READY TO MERGE
No blocking issues found
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | Low | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- Medium: 1 issue(s)
- Low: 3 issue(s)
Generated by Auto Claude PR Review
Findings (4 selected of 4 total)
🟡 [MEDIUM] getPythonVersion still uses execSync with string interpolation
📁 apps/frontend/src/main/python-detector.ts:130
The getPythonVersion function at line 130 still uses execSync(${pythonCmd} --version) which could fail for paths containing spaces. This is inconsistent with the PR's goal of fixing space handling. While this function is primarily called with simple command names, the pattern should be updated for consistency with the execFileSync approach used elsewhere.
Suggested fix:
Update getPythonVersion to use parsePythonCommand() and execFileSync similar to verifyIsPython(): `const [cmd, args] = parsePythonCommand(pythonCmd); const version = execFileSync(cmd, [...args, '--version'], { stdio: 'pipe', timeout: 5000, windowsHide: true, shell: false }).toString().trim();`
🔵 [LOW] Edge case regression for files with spaces in name
📁 apps/frontend/src/main/python-detector.ts:234
The new logic order (checking path separators before existsSync) introduces a theoretical regression: if a user has an actual file literally named 'py -3' in the current directory, it would now be incorrectly split into ['py', ['-3']] instead of being recognized as a single file. This is an extremely unlikely scenario in practice.
Suggested fix:
Consider adding a comment documenting this behavioral change, or add a final existsSync check for non-path-like strings before splitting. Given the low probability of this edge case, this is acceptable for now.
🔵 [LOW] Regex patterns use permissive character class for app names
📁 apps/frontend/src/main/python-detector.ts:305
The new regex patterns use [^/]+ and [^\\]+ which allow any characters except path separators in the username and app name positions. While this works correctly and the existing path.normalize() and '..' check provide protection, using a more restrictive character class like [a-zA-Z0-9_\-. ]+ would reduce attack surface.
Suggested fix:
Consider tightening the regex to `/^\/Users\/[a-zA-Z0-9_\-. ]+\/Library\/Application Support\/[a-zA-Z0-9_\-. ]+\/python-venv\/bin\/python\d*(\.\d+)?$/` for defense in depth. The current pattern is acceptable given the existing validation layer.
🔵 [LOW] No unit tests for parsePythonCommand with space-containing paths
📁 apps/frontend/src/main/python-detector.ts:215
The PR adds critical logic for handling paths with spaces, but there are no unit tests specifically testing scenarios like '/Users/user/Library/Application Support/App/python-venv/bin/python'. The existing subprocess-spawn.test.ts tests the spawning but not the path parsing edge cases.
Suggested fix:
Add unit tests for parsePythonCommand covering: 1) paths with spaces that exist, 2) paths with spaces that don't exist yet, 3) simple commands like 'py -3', 4) Windows paths with spaces. This ensures the fix doesn't regress.
This review was generated by Auto Claude.
|
I have read the CLA Document and I hereby sign the CLA |
- Add missing parsePythonCommand import (CodeRabbit critical) - Consolidate macOS Application Support regex patterns (Gemini) - Consolidate Windows AppData Roaming regex patterns (Gemini) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
AlexMadera
left a comment
AlexMadera
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: ✅ READY TO MERGE
No blocking issues found
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | Low | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- Medium: 1 issue(s)
- Low: 3 issue(s)
Generated by Auto Claude PR Review
No findings selected for this review.
This review was generated by Auto Claude.
MikeeBuilds
added
bug
…unction - Add ShellType union type for supported shells: powershell, cmd, bash, zsh, fish, sh - Add detectShellType(shellPath) function to detect shell from executable path - Supports Windows PowerShell 5.1, PowerShell Core 7+, cmd.exe, Git Bash, WSL bash, zsh, fish - Defaults to 'bash' for unknown shells (POSIX-compatible fallback) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
… detectShellType Add comprehensive unit tests for the detectShellType() function: - PowerShell detection (Windows PowerShell 5.1, pwsh, case insensitivity) - cmd.exe detection (with and without extension) - Bash detection (standard paths, Git Bash, WSL) - Zsh detection - Fish shell detection - Bourne shell (sh) detection - Default fallback to bash for unknown shells 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…h all shell types Adds comprehensive unit tests for buildCdCommandForShell() covering: - Undefined/empty path handling for all shell types - bash/zsh/fish/sh: POSIX single-quote escaping with '\'' for quotes - PowerShell: Set-Location with single quotes and '' escape for quotes - cmd.exe: cd /d with double quotes and ^ escape for special chars - Edge cases: root paths, drive roots, multiple quotes, special chars 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Added comprehensive unit tests for the escapeShellArgPowerShell function: - Basic escaping: simple strings, empty string, spaces, paths - Single quote escaping: single, multiple, start/end positions, consecutive - Special character handling: variables, command substitution, backticks - Security edge cases: command injection attempts, variable expansion - Path edge cases: UNC paths, parentheses, brackets, curly braces 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…ell type - Add optional shellType parameter to invokeClaude() function signature - Import buildCdCommandForShell and ShellType from shell-escape utilities - Use shell-appropriate cd command syntax when shellType is provided - Add JSDoc documentation for the new parameter - Add debug logging for shell type being used 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
… shell awareness Added shell-aware command generation for profile-based Claude invocation with helpers for buildClearCommand, buildConfigDirCommand, and buildTokenCommand supporting PowerShell, cmd.exe, and POSIX shells. 🤖 Generated with Claude Code Co-Authored-By: Claude Opus 4.5 <[email protected]>
…handlers Updated CLAUDE_PROFILE_INITIALIZE handler to be shell-aware: - Import detectShellType, escapeShellArgPowerShell, and ShellType from shell-escape.ts - Detect shell type using same logic as pty-manager (COMSPEC/SHELL env vars) - Generate shell-appropriate login commands for PowerShell, cmd.exe, fish, and POSIX shells - Use proper escaping for each shell type to prevent command injection 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add shellType property to TerminalProcess interface to track the detected shell type for each terminal. This enables shell-aware command generation when invoking Claude or running profile login commands. - Import ShellType from shell-escape.ts - Add optional shellType property with JSDoc comment 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…ash/zsh
- Added 9 backward compatibility tests for buildCdCommand():
- Verify function exists and is exported
- Test undefined/empty path handling
- Test basic path escaping with single quotes
- Test POSIX single quote escaping (')
- Test paths with spaces
- Test paths with special characters ($HOME)
- Verify identical output to buildCdCommandForShell(path, 'bash')
Manual verification completed:
- buildCdCommand() still exists in shell-escape.ts (lines 109-114)
- Fallback to buildCdCommand() when shellType not provided
(claude-integration-handler.ts lines 352-354)
- Output equivalence confirmed: both use escapeShellArg() internally
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
SECURITY FIXES: - Token temp file: Use single quotes with POSIX escaping instead of unescaped double quotes to prevent command injection if token contains shell metacharacters like $() or backticks - Add newline/CR stripping to all escape functions to prevent multi-line command injection attacks - escapeShellArg (POSIX): Now strips \r\n before escaping - escapeShellArgPowerShell: Now strips \r\n before escaping - escapeShellArgWindows: Now strips \r\n before escaping TEST COVERAGE: - Add 65+ new tests for escapeShellArg, escapeShellArgWindows, and isPathSafe functions that were previously untested - Add newline injection prevention tests for all escape functions - Add command injection security tests for POSIX escaping - Total tests: 149 (all passing) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Security Fixes Added (Adversarial Analysis)After running adversarial prompting analysis on the code, I identified and fixed the following security issues: 🔴 CRITICAL: Token temp file vulnerability (FIXED)File: Before (vulnerable): fs.writeFileSync(tempFile, `export CLAUDE_CODE_OAUTH_TOKEN="${token}"\n`, ...)After (fixed): const escapedTokenForExport = token.replace(/'/g, "'\\''");
fs.writeFileSync(tempFile, `export CLAUDE_CODE_OAUTH_TOKEN='${escapedTokenForExport}'\n`, ...)If a token contained shell metacharacters like 🟠 HIGH: Newline injection prevention (FIXED)All escape functions now strip
📊 Test Coverage AddedAdded 65+ new tests covering:
Total tests: 149 (all passing) |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
apps/frontend/src/main/ipc-handlers/terminal-handlers.tsapps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/main/terminal/types.tsapps/frontend/src/shared/utils/shell-escape.test.tsapps/frontend/src/shared/utils/shell-escape.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/terminal/types.tsapps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/main/ipc-handlers/terminal-handlers.tsapps/frontend/src/shared/utils/shell-escape.tsapps/frontend/src/shared/utils/shell-escape.test.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/terminal/types.tsapps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/main/ipc-handlers/terminal-handlers.tsapps/frontend/src/shared/utils/shell-escape.tsapps/frontend/src/shared/utils/shell-escape.test.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/terminal/types.tsapps/frontend/src/main/terminal/claude-integration-handler.tsapps/frontend/src/main/ipc-handlers/terminal-handlers.tsapps/frontend/src/shared/utils/shell-escape.tsapps/frontend/src/shared/utils/shell-escape.test.ts
🧬 Code graph analysis (3)
apps/frontend/src/main/terminal/types.ts (1)
apps/frontend/src/shared/utils/shell-escape.ts (1)
ShellType(12-12)
apps/frontend/src/main/terminal/claude-integration-handler.ts (1)
apps/frontend/src/shared/utils/shell-escape.ts (6)
ShellType(12-12)escapeShellArgPowerShell(209-216)escapeShellArgWindows(172-190)escapeShellArg(86-93)buildCdCommandForShell(137-160)buildCdCommand(111-116)
apps/frontend/src/main/ipc-handlers/terminal-handlers.ts (1)
apps/frontend/src/shared/utils/shell-escape.ts (5)
ShellType(12-12)detectShellType(33-68)escapeShellArgPowerShell(209-216)escapeShellArgWindows(172-190)escapeShellArg(86-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (13)
apps/frontend/src/shared/utils/shell-escape.ts (5)
8-68: LGTM! Comprehensive shell detection with sensible defaults.The
detectShellTypefunction correctly identifies common shells across platforms (Windows PowerShell, pwsh, cmd.exe, Git Bash, WSL, zsh, fish, sh) and defaults tobashfor unknown shells, which is a safe POSIX-compatible fallback.
86-93: Good security hardening: newline sanitization prevents command injection.Stripping
\rand\nbefore escaping is a solid defense-in-depth measure against newline-based command injection attacks.
172-190: LGTM! Correct cmd.exe escaping with proper metacharacter handling.The escaping order (caret first, then other metacharacters) and doubling of
%for variable expansion prevention are correct for cmd.exe.
192-216: LGTM! Correct PowerShell single-quote escaping.Doubling single quotes (
'→'') is the correct escape mechanism for PowerShell literal strings, and newline sanitization adds security.
152-158: The current implementation is correct for fish shell. The POSIX escaping pattern'\''used inescapeShellArg()works across all POSIX shells including fish because it deliberately exits the quoted context to perform the escape. While fish does support an alternative escape syntax (\'inside single quotes), the current pattern remains valid: the first'closes the current quoted string,\'escapes the quote outside any quoting context (which fish supports), and the final'reopens quoting. This approach is universally compatible with bash, zsh, fish, and sh.apps/frontend/src/main/terminal/types.ts (1)
18-19: LGTM! Clean interface extension with proper documentation.The optional
shellTypeproperty is well-documented and maintains backward compatibility with existing code.apps/frontend/src/main/ipc-handlers/terminal-handlers.ts (1)
336-379: LGTM! Well-implemented shell-aware command construction.The shell detection and command generation correctly handle:
- PowerShell:
$env:VAR = value; command- cmd.exe:
set "VAR=value" && command- fish:
set -x VAR value; and command(correct fish syntax)- POSIX shells:
export VAR=value && commandSecurity is maintained through shell-specific escaping functions.
apps/frontend/src/shared/utils/shell-escape.test.ts (2)
294-309: Verify fish shell escaping test expectations.These tests assume fish uses the same
'\''escaping as POSIX shells. If fish handles single quotes differently (as noted in the shell-escape.ts review), these test expectations may be incorrect and should be updated to match fish's actual behavior.
1-833: Excellent test coverage for shell-escape utilities.The test suite comprehensively covers:
- Shell type detection across all supported shells
- Backward compatibility with
buildCdCommand- Shell-specific command generation for bash, zsh, fish, sh, PowerShell, and cmd
- Security edge cases including command injection, newline injection, and special character handling
- Path edge cases (UNC paths, parentheses, brackets, drive roots)
The equivalence tests between
buildCdCommandandbuildCdCommandForShell('bash')ensure backward compatibility.apps/frontend/src/main/terminal/claude-integration-handler.ts (4)
205-224: LGTM! Shell-appropriate clear commands.Correctly uses
Clear-Hostfor PowerShell,clsfor cmd, andclearfor POSIX shells with appropriate command separators.
250-260: Consider: fish shell usesbash -csubprocess.For fish shell, the code falls through to the POSIX case which uses
bash -c 'exec claude'. This works but spawns a bash subprocess instead of running natively in fish. This is acceptable since fish can't easily source bash-style export files, but worth noting for debugging if users report issues with fish.
314-321: LGTM! Well-documented optional parameter with backward compatibility.The
shellTypeparameter is optional and the function correctly falls back to POSIX-style commands when not specified, maintaining backward compatibility.
370-395: Good security design: temp file for POSIX, direct env for Windows shells.The branching logic correctly uses:
- Temp file method for POSIX shells (avoids token in command line/history)
- Direct environment variable setting for PowerShell/cmd (these shells don't have
sourceequivalent, and the escaping functions handle security)This is a sensible security/compatibility tradeoff.
| // SECURITY: Use single quotes to prevent any variable expansion or command substitution | ||
| // Escape any single quotes in the token using POSIX escaping: ' → '\'' | ||
| const escapedTokenForExport = token.replace(/'/g, "'\\''"); | ||
| fs.writeFileSync(tempFile, `export CLAUDE_CODE_OAUTH_TOKEN='${escapedTokenForExport}'\n`, { mode: 0o600 }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using escapeShellArg for consistency.
The manual POSIX escaping token.replace(/'/g, "'\\''") duplicates the logic in escapeShellArg. Consider reusing the utility, though note that escapeShellArg also strips newlines and wraps in quotes, so the current approach is correct for the export statement context.
🔎 Optional refactor for consistency
- // SECURITY: Use single quotes to prevent any variable expansion or command substitution
- // Escape any single quotes in the token using POSIX escaping: ' → '\''
- const escapedTokenForExport = token.replace(/'/g, "'\\''");
- fs.writeFileSync(tempFile, `export CLAUDE_CODE_OAUTH_TOKEN='${escapedTokenForExport}'\n`, { mode: 0o600 });
+ // SECURITY: Use escapeShellArg which handles single quotes and strips newlines
+ const escapedToken = escapeShellArg(token);
+ fs.writeFileSync(tempFile, `export CLAUDE_CODE_OAUTH_TOKEN=${escapedToken}\n`, { mode: 0o600 });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SECURITY: Use single quotes to prevent any variable expansion or command substitution | |
| // Escape any single quotes in the token using POSIX escaping: ' → '\'' | |
| const escapedTokenForExport = token.replace(/'/g, "'\\''"); | |
| fs.writeFileSync(tempFile, `export CLAUDE_CODE_OAUTH_TOKEN='${escapedTokenForExport}'\n`, { mode: 0o600 }); | |
| // SECURITY: Use escapeShellArg which handles single quotes and strips newlines | |
| const escapedToken = escapeShellArg(token); | |
| fs.writeFileSync(tempFile, `export CLAUDE_CODE_OAUTH_TOKEN=${escapedToken}\n`, { mode: 0o600 }); |
🤖 Prompt for AI Agents
In apps/frontend/src/main/terminal/claude-integration-handler.ts around lines
380-383, replace the manual POSIX escaping token.replace(/'/g, "'\\''") with the
shared escapeShellArg utility for consistency: call escapeShellArg(token) and
use its return value directly in the export assignment (it already wraps in
quotes and escapes single quotes), updating the fs.writeFileSync call
accordingly; ensure this change is acceptable given escapeShellArg strips
newlines—if preserving newlines is required, keep the current manual replace but
add a comment referencing escapeShellArg and why manual escaping is necessary.
- Use parsePythonCommand + execFileSync in getPythonVersion (fixes MEDIUM) - Add /root/.pyenv pattern for Linux root user (from adversarial analysis) - Remove unused execSync import - Tighten regex patterns: pyenv/conda require explicit home directories - Add 23 unit tests for parsePythonCommand and validatePythonPath 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/main/python-detector.ts (1)
319-321: Windows venv patterns are overly permissive compared to Unix equivalents.These patterns use
^.*\\prefix which allows any path, including potentially malicious locations (e.g.,C:\attacker\controlled\.venv\Scripts\python.exe). The Unix venv patterns (lines 312-314) are more restrictive, requiring alphanumeric directory names.🔎 Proposed fix to constrain Windows venv patterns
// Windows virtual environments - /^.*\\\.?venv\\Scripts\\python\.exe$/i, - /^.*\\\.?virtualenv\\Scripts\\python\.exe$/i, - /^.*\\env\\Scripts\\python\.exe$/i, + // Only allow alphanumeric, dash, underscore, dot, and space in directory names + /^(?:[A-Za-z]:\\)?(?:[a-zA-Z0-9._\- ]+\\)+\.?venv\\Scripts\\python\.exe$/i, + /^(?:[A-Za-z]:\\)?(?:[a-zA-Z0-9._\- ]+\\)+\.?virtualenv\\Scripts\\python\.exe$/i, + /^(?:[A-Za-z]:\\)?(?:[a-zA-Z0-9._\- ]+\\)+env\\Scripts\\python\.exe$/i,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/frontend/src/main/__tests__/python-detector.test.tsapps/frontend/src/main/python-detector.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/main/__tests__/python-detector.test.tsapps/frontend/src/main/python-detector.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/main/__tests__/python-detector.test.tsapps/frontend/src/main/python-detector.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/__tests__/python-detector.test.tsapps/frontend/src/main/python-detector.ts
🧬 Code graph analysis (1)
apps/frontend/src/main/__tests__/python-detector.test.ts (1)
apps/frontend/src/main/python-detector.ts (2)
parsePythonCommand(222-278)validatePythonPath(414-503)
🪛 GitHub Actions: CI
apps/frontend/src/main/__tests__/python-detector.test.ts
[error] 3-3: Cannot find name 'describe'. Do you need to install type definitions for a test runner? Try 'npm i --save-dev @types/jest' or 'npm i --save-dev @types/mocha'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (7)
apps/frontend/src/main/__tests__/python-detector.test.ts (1)
97-150: Good security test coverage for validatePythonPath.The tests comprehensively cover dangerous shell metacharacters (
$(), backticks,;,|, newlines), path traversal, and null/empty inputs. This aligns well with the security hardening in the PR.Consider adding tests for mixed-quote scenarios (e.g.,
"/path/to/'python") and Windows UNC paths if those are expected inputs.apps/frontend/src/main/python-detector.ts (6)
129-145: Good security improvement using execFileSync with shell: false.Using
parsePythonCommandto split the command and args, then executing withexecFileSyncandshell: falsecorrectly addresses the spawn ENOENT issue for paths with spaces while preventing shell injection.
240-258: Core fix correctly implemented: path separators checked before existence.The reordering to check
hasPathSeparatorsbeforeexistsSync(lines 240-258) correctly addresses the root cause where non-existent venv paths were incorrectly split on spaces. The detailed comments explain the rationale well.
363-367: Dual path testing handles cross-platform patterns correctly.Testing both the original path and the normalized path ensures Windows patterns (with
\) and Unix patterns (with/) both match appropriately.
385-400: Consistent secure execution pattern in verifyIsPython.Uses the same
parsePythonCommand+execFileSyncwithshell: falsepattern asgetPythonVersion, maintaining consistent security posture.
293-293: Comprehensive dangerous character detection including newlines.The regex correctly covers shell metacharacters for command injection (
; | &), subshells ($ ( ) \``), redirects (< >), globs (* ? [ ]), and newline injection (\n \r`). This aligns with the security fixes mentioned in the PR comments.
452-461: Path traversal check is effective; reasoning in original comment was incorrect.The concern about
path.normalize()was based on a misunderstanding. Whilepath.normalize()does resolve..segments in absolute paths, it does NOT fully resolve relative paths—relative traversal sequences like../../etc/passwdremain in the normalized string and are caught by theincludes('..')check.For absolute paths that attempt traversal, the strict allowlist at line 464 provides the primary defense. All 30+ patterns are absolute paths for known safe locations (system Python, Homebrew, pyenv, virtual environments, conda). A path that escapes these directories would fail the allowlist check regardless.
The security design is sound: relative traversal is caught by the string check, and absolute paths are restricted to allowlisted locations.
| import { parsePythonCommand, validatePythonPath } from '../python-detector'; | ||
|
|
||
| describe('parsePythonCommand', () => { |
There was a problem hiding this comment.
Add Jest type definitions to fix pipeline failure.
The CI pipeline reports Cannot find name 'describe'. The test file needs Jest type definitions to compile.
🔎 Proposed fix
Add the import at the top of the file or install @types/jest:
+/// <reference types="jest" />
import { parsePythonCommand, validatePythonPath } from '../python-detector';
describe('parsePythonCommand', () => {Alternatively, ensure @types/jest is installed and included in tsconfig.json:
npm i --save-dev @types/jest📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { parsePythonCommand, validatePythonPath } from '../python-detector'; | |
| describe('parsePythonCommand', () => { | |
| /// <reference types="jest" /> | |
| import { parsePythonCommand, validatePythonPath } from '../python-detector'; | |
| describe('parsePythonCommand', () => { |
🧰 Tools
🪛 GitHub Actions: CI
[error] 3-3: Cannot find name 'describe'. Do you need to install type definitions for a test runner? Try 'npm i --save-dev @types/jest' or 'npm i --save-dev @types/mocha'.
🤖 Prompt for AI Agents
In apps/frontend/src/main/__tests__/python-detector.test.ts around lines 1 to 3,
the test file fails to compile because Jest globals like "describe" are not
recognized; fix by either adding Jest type definitions to the file (e.g., add a
single line at the top referencing Jest types) or install and enable @types/jest
project-wide (npm i --save-dev @types/jest) and ensure tsconfig.json includes
"jest" in the "types" array or the test tsconfig includes the Jest types so the
compiler recognizes Jest globals.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/frontend/src/shared/utils/shell-escape.ts (1)
112-126: Potential inconsistency:buildCdCommanduses platform detection whilebuildCdCommandForShelluses shell type.
buildCdCommandassumeswin32platform means cmd.exe, but users on Windows may run PowerShell or Git Bash. Consider deprecating this function in favor ofbuildCdCommandForShellwhich uses explicit shell type detection, or update it to detect the shell type.apps/frontend/src/main/python-detector.ts (3)
309-311: Windows venv patterns use overly permissive.*prefix.Lines 309-311 use
^.*\\which matches any prefix, potentially allowing malicious paths. Consider constraining to known safe locations similar to the Unix patterns.🔎 Suggested stricter Windows venv patterns
// Windows virtual environments - /^.*\\\.?venv\\Scripts\\python\.exe$/i, - /^.*\\\.?virtualenv\\Scripts\\python\.exe$/i, - /^.*\\env\\Scripts\\python\.exe$/i, + // Windows virtual environments - constrained to user directories + /^[A-Za-z]:\\Users\\[^\\]+\\[^\\]+\\\.?venv\\Scripts\\python\.exe$/i, + /^[A-Za-z]:\\Users\\[^\\]+\\[^\\]+\\\.?virtualenv\\Scripts\\python\.exe$/i, + /^[A-Za-z]:\\Users\\[^\\]+\\[^\\]+\\env\\Scripts\\python\.exe$/i,
355-359: Path normalization may cause regex mismatch on Windows.
matchesAllowedPatternnormalizes backslashes to forward slashes for matching, but most Windows patterns inALLOWED_PATH_PATTERNSexpect backslashes. The function tests both the original and normalized path, which should work, but this dual-testing approach is fragile.Consider normalizing all patterns to use forward slashes, or explicitly document which patterns expect which separator style:
🔎 Suggested clarification
function matchesAllowedPattern(pythonPath: string): boolean { - // Normalize path separators for consistent matching + // Test both original path (for Windows backslash patterns) and + // normalized path (for Unix forward-slash patterns) const normalizedPath = pythonPath.replace(/\\/g, '/'); return ALLOWED_PATH_PATTERNS.some(pattern => pattern.test(pythonPath) || pattern.test(normalizedPath)); }
497-509: Consider logging successful validations for debugging.Currently only invalid paths are logged. Consider adding a debug log for successful validations to aid troubleshooting path-with-spaces issues in production.
🔎 Suggested logging improvement
export function getValidatedPythonPath(providedPath: string | undefined, serviceName: string): string { if (!providedPath) { return findPythonCommand() || 'python'; } const validation = validatePythonPath(providedPath); if (validation.valid) { + console.log(`[${serviceName}] Validated Python path: ${validation.sanitizedPath}`); return validation.sanitizedPath || providedPath; } console.error(`[${serviceName}] Invalid Python path rejected: ${validation.reason}`); return findPythonCommand() || 'python'; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/frontend/src/main/python-detector.tsapps/frontend/src/shared/utils/shell-escape.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/frontend/src/**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use i18n translation keys for all user-facing text in the frontend instead of hardcoded strings
Files:
apps/frontend/src/shared/utils/shell-escape.tsapps/frontend/src/main/python-detector.ts
apps/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
useTranslation()hook with namespace prefixes (e.g., 'navigation:items.key') for accessing translation strings in React components
Files:
apps/frontend/src/shared/utils/shell-escape.tsapps/frontend/src/main/python-detector.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/shared/utils/shell-escape.tsapps/frontend/src/main/python-detector.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (11)
apps/frontend/src/shared/utils/shell-escape.ts (5)
8-68: LGTM! Shell type detection is well-structured.The
detectShellTypefunction correctly identifies various shell types with reasonable defaults. The order of checks is appropriate (more specific patterns before generic ones), and defaulting tobashfor unknown shells is a sensible POSIX-compatible fallback.
86-93: Good security fix: newline/CR sanitization prevents command injection.Stripping
\r\nbefore escaping single quotes closes a command injection vector where an attacker could break out of single-quoted strings using newlines. This is a solid defense-in-depth approach.
147-170: LGTM! Shell-specific cd command generation is well-implemented.The function correctly uses:
Set-Locationwith semicolon separator for PowerShellcd /dwith&separator for cmd.exe (supporting drive changes)- Standard
cdwith&&for POSIX shellsEach shell type uses its appropriate escaping function.
182-200: Good security hardening with newline removal.The addition of
\rand\nremoval (lines 196-197) closes a command injection vector in cmd.exe. The order of escape operations is correct—carets are escaped first since^is the escape character.
219-226: LGTM! PowerShell escaping is correct.PowerShell single-quoted strings only require doubling single quotes (
'→''), and newline sanitization is properly applied before escaping.apps/frontend/src/main/python-detector.ts (6)
1-1: Good security improvement: switching toexecFileSync.Using
execFileSyncwithshell: falseprevents shell injection attacks when executing Python commands, especially important for paths containing spaces or special characters.
119-135: LGTM! Safe Python version extraction.Using
execFileSyncwithshell: falseandparsePythonCommandcorrectly handles paths with spaces without shell interpretation issues.
230-268: LGTM! Correct reordering fixes the path-with-spaces issue.The key fix: checking for path separators (
/or\) beforeexistsSyncensures that paths like/Users/user/Library/Application Support/App/python-venv/bin/pythonare treated as single paths rather than split on spaces, even when the venv doesn't exist yet during initial setup.The logic flow is now:
- Has path separators and doesn't start with
-→ treat as path (no split)- File exists without separators → treat as executable in current directory
- Otherwise → split on spaces for commands like
py -3This correctly handles the root cause described in the PR.
377-392: LGTM! Safe Python verification.Using
execFileSyncwithshell: falseand verifying the output matchesPython X.Yis a solid defense-in-depth measure to ensure the executable is actually Python.
406-495: Comprehensive validation with good layered security.The validation flow is well-structured:
- Shell metacharacter rejection
- Safe command allowlist check
- Path traversal detection
- Allowlist pattern matching
- Existence and executability checks
- Runtime Python verification
This defense-in-depth approach significantly reduces command injection risk.
296-307: The regex patterns work correctly; the concern about unescaped spaces is based on a misunderstanding of regex syntax.Spaces inside character classes
[ ]do not require escaping and are literal characters in regex. The test results confirm all patterns match their intended paths:
- Virtual environment patterns correctly match
/home/user/my-project/.venv/bin/pythonpaths- macOS Application Support pattern correctly matches
/Users/*/Library/Application Support/*/venv/bin/pythonpaths- Character classes with spaces function as intended
The patterns are internal path validation logic, not user-facing text, so no changes are needed.
Likely an incorrect or invalid review comment.
5 participants
Summary
parsePythonCommandto check for path separators BEFOREexistsSyncto properly handle paths that may not exist yetALLOWED_PATH_PATTERNSfor Electron userData venv pathspython-env-managerto useexecFileSyncinstead ofexecSyncto avoid shell interpretation issues with spacesProblem
When using the packaged app on macOS, the venv Python path contains spaces:
/Users/user/Library/Application Support/App/python-venv/bin/pythonThis was causing
spawn ENOENTerrors during merge operations because the path was being incorrectly split at the space character.Root Cause
The
parsePythonCommandfunction was checkingexistsSync()before checking if the path looked like a file path. If the file didn't exist (e.g., during initial setup), and the path wasn't recognized as a path, it would fall through to splitting on spaces.Fix
/or\) FIRST, before checking file existence/Users/<user>/Library/Application Support/<app>/python-venv/bin/pythonC:\Users\<user>\AppData\Roaming\<app>\python-venv\Scripts\python.exepython-env-manager.tsto useexecFileSyncwithshell: falseto avoid shell interpretation issuesTest Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.